Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS Support #4

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

TLS Support #4

wants to merge 7 commits into from

Conversation

bdoona
Copy link

@bdoona bdoona commented Feb 29, 2024

Adds TLS support to mqtt connections.

I started working on this before seeing the other pull request. Hopefully this one can be accepted.

Please let me know if any changes need to be made.

@lbt
Copy link
Owner

lbt commented Mar 1, 2024

overall this looks good. I'll test and merge when I have a little time.

self.mqtt_port = 1883
# Set ssl
try:
self.mqtt_usessl = self.strtobool(settings.MQTT_USE_SSL)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this? it seems broken.
settings.MQTT_USE_SSL is a bool already.

Please remove the strtobool() method and document it as being optional and taking True/False values.

README.md Outdated
MQTT_USE_SSL = True # Enable ssl connection
## Stop here if your certificate has been properly signed.

## Settings for self-signed certificates
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be mixing up self-signed certs and client certs.
This seems to be for using client certs.

If the server is self-signed then in theory you could pass in something to the loop.create_connection() but qmqtt doesn't support it (and it's not worth having IMHO).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant self-signed certs on the server. I'll amend the comment appropriately.

self.mqtt_ssl_key = settings.MQTT_SSL_KEY
try:
self.mqtt_ssl_verify = \
self.strtobool(settings.MQTT_SSL_VERIFY)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove strtobool()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a valid use case and a story behind this...

The use case

.env file

# deployment specific secrets and settings
# --- snip ---

# mqtt settings
# host, username, password, etc
# including...
MQTT_PORT=8883
MQTT_USE_SSL=True
MQTT_SSL_VERIFY=True

My django settings.py file

# other includes
# --- snip ---

from dotenv import load_dotenv
load_dotenv()
env = os.environ

# --- snip ---
# standard django settings.py stuff
# --- snip ---

MQTT_PORT=env.get('MQTT_PORT', None)
MQTT_USE_SSL=env.get('MQTT_USE_SSL', None)
MQTT_SSL_VERIFY=env.get('MQTT_SSL_VERIFY', None)

By the time they reach channelsmqttproxy.py:

type(settings.MQTT_PORT) = <class 'str'>
type(settings.MQTT_USE_SSL) = <class 'str'>
type(settings.MQTT_SSL_VERIFY) = <class 'str'>

The story

Now, the obvious stuff

int(2) = 2
int('2') = 2
bool(True) = True
bool('True') = True
bool(False) = False

but, here's the thing that got me...

bool('False') = True # Makes sense when you stop and think about it but doesn't seem intuitive for this case

Proposal

Above these blocks with the strtobool() calls, there is already a call to int() to ensure MQTT_PORT is an int or can be converted to an int.
Given that there is already protection for MQTT_PORT, I would like to include the same for MQTT_USE_SSL and MQTT_SSL_VERIFY.

Having said all that, I would like to

  • Rename strtobool() to tobool() to be closer to its intent.
  • Have tobool() behave in the same manner as int() would i.e.
tobool(True) = True
tobool('True') = True
tobool(False) = False
tobool('False') = False # Fixed for intent

Please let me know if you are happy with that and I will proceed with the changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and I'll remember to run it through the linter this time too.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the issue - however I think the settings.py should be documented as taking bool.
The solution would be to have this in settings.py:

   from mysettings.helper import tobool
   
    MQTT_USE_SSL = tobool(env.get('MQTT_USE_SSL', None))

Doesn't that make more sense?

@lbt
Copy link
Owner

lbt commented Mar 2, 2024

I've set up my MQTT with TLS and tested. It all seems to work :)
A few comments on the code. There are a couple of pylint issues too.

I have not tested the client cert stuff yet but I will.

All submitted setting variables must be of the correct type.

References:
* lbt#4 (comment)
* lbt#4 (comment)
* Use lazy % formatting in logging functions Pylint (W1203:logging-fstring-interpolation)
* Using deprecated method warn() Pylint (W4902:deprecated-method)
* Catching too general exception Exception Pylint (W0718:broad-exception-caught)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants